Repository navigation
Memory regression tests - #275
alan-george-lk wants to merge 13 commits into
Conversation
2a21a8a to
d638cca
Compare
…ature-memory-regression-tests
![devin-ai-integration[bot]](https://avatars.githubusercontent.com/in/811515?s=60&v=4){kind=small}
There was a problem hiding this comment.
Note
Newer findings are available below. Devin Review posted a newer report on this PR, in addition to the findings presented here.
🔍 Devin Review: 2 flags
Not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
There was a problem hiding this comment.
Devin Review found 2 new potential issues.
2 flags not posted on this PR by your GitHub settings — view them in Devin Review. (Configure)
| if (current.process.threads > warmup.process.threads) { | ||
| first_thread_growth_iteration = iteration; | ||
| first_thread_growth = std::move(current); |
There was a problem hiding this comment.
🟡 Transient thread obscures leak start
When a transient worker raises the count after warmup, first_thread_growth_iteration latches that cycle. If the worker exits before a later leak, the final diagnostic reports the transient cycle as the growth onset.
Learn more
The diagnostic records the first cycle whose thread count exceeds the quiescent warmup count. This is a one-time latch: the final report never checks whether the extra thread survived. A temporary teardown worker can therefore take the place of the first persistent increase in a run that eventually fails the thread gate.
Example: Warmup has three threads. Cycle 21 briefly has four, cycle 22 returns to three, and a leaked worker first appears at cycle 90. The final count is four, but the diagnostic says growth first appeared after cycle 21.
Recommended fix: Track transitions back to the warmup count and record the start of the current sustained growth interval, or label the recorded iteration explicitly as the first transient count increase rather than the leak onset.
Was this helpful? React with 👍 or 👎 to provide feedback.
| if (next.process.threads != snapshot.process.threads) { | ||
| stable_since = std::chrono::steady_clock::now(); | ||
| } else if (std::chrono::steady_clock::now() - stable_since >= kThreadQuiescencePeriod) { | ||
| return next; |
There was a problem hiding this comment.
🟡 Thread churn passes quiescence check
When one worker exits as another starts, sampleQuiescentProcess sees an unchanged thread count and declares quiescence. A checkpoint can include active teardown workers, giving the leak gate a false baseline or final count.
Learn more
Checkpoint sampling waits for a stable number of threads, but equal totals do not mean that the same threads remained alive. The cycle loop runs connection and track teardown repeatedly, so short-lived workers are precisely what this check is meant to exclude. Workers that are replaced between polls leave the total unchanged and allow a checkpoint while teardown is still running.
Example: A checkpoint starts with three threads. On successive polls one teardown worker exits while another starts; every poll still sees three. After 500 ms the function accepts the snapshot even though none of the sampled workers has survived the entire period.
Recommended fix: Track the set of thread IDs from /proc/self/task across polls and reset the stable interval when membership changes; keep thread names only for reporting.
Was this helpful? React with 👍 or 👎 to provide feedback.
1 participant
This PR introduces a new test binary called
memory_testerfor Linux, which has the following features:CI is also updated to run this tester and provide a nice markdown table report of the results, and fail jobs that introduce changes that fail this.
Documentation is also updated around testing in general, as it was a bit out of date.
Results
Here are two FFI release results with the following params:
Prior release
livekit-ffi/v0.12.81results:Current FFI release results: